Skip to content

Add unionTypes and interfaceTypes to the json-modern IR#2050

Merged
hwillson merged 1 commit intomasterfrom
union-interface-types
Jul 30, 2020
Merged

Add unionTypes and interfaceTypes to the json-modern IR#2050
hwillson merged 1 commit intomasterfrom
union-interface-types

Conversation

@hwillson
Copy link
Copy Markdown
Member

This commit adds new top level properties called unionTypes and interfaceTypes, to the json-modern exported IR. These properties list all unions and their types, as well as all interfaces and their implementing types.

@hwillson hwillson force-pushed the union-interface-types branch from 14c4570 to 2def792 Compare July 24, 2020 18:23
@hwillson hwillson self-assigned this Jul 24, 2020

this.typesUsedSet = new Set();
this.unionTypesSet = new Set();
this.interfaceTypesMap = new Map();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are the union types a set and the interface types a map?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@designatednerd There's a slight difference in the details that union types and interface types give us access to. For the union types, we want to list the union type name, and the types that make up that union. graphql-js supports this out of the box by giving GraphQLUnionType's a types property, which holds the array of types that make up the union. Because of this, we can just store the GraphQLUnionType's in a Set. When the serializeToJSON function is then called to write out the IR as JSON, we can access the union types we want to show by calling getTypes() on each GraphQLUnionType itself.

GraphQLInterfaceType's are handled a bit differently though. graphql-js doesn't provide a types property (or something similar) that gives us easy access to every type using a specific interface. We have to get this ourselves. There was already a helper method setup in the Compiler class (in index.ts) to do this, called possibleTypesForType. Since it's already there in the compiler, I think it makes sense to leverage it, and pass both the interface type, and the types using that interface, through to the serializeToJSON function using a Map.

I'm happy to handle this differently of course. Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was mostly curious, thanks for the explanation

\\"name\\": \\"Character\\",
\\"types\\": [
\\"Human\\",
\\"Droid\\"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there also an Alien type? Or is this going to only have the possible types that are actually used?

Copy link
Copy Markdown
Member Author

@hwillson hwillson Jul 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@designatednerd The schema the tests are using doesn't have an Alien type. You can see it here.

This commit adds new top level properties called `unionTypes` and
`interfaceTypes`, to the `json-modern` exported IR. These properties
list all unions and their types, as well as all interfaces and their
implementing types.
@hwillson hwillson force-pushed the union-interface-types branch from 2def792 to 54c60ed Compare July 24, 2020 20:16
Copy link
Copy Markdown
Contributor

@jbaxleyiii jbaxleyiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hwillson one thing we don't support yet is interfaces implementing interfaces for iOS codegen. The CLI was recently updated to support graphql-js 15 where this feature landed if we want to try and add that to a schema for a test and support it here. However I wouldn't block on it if you didn't want to as we can come back to it later

@hwillson hwillson merged commit 512fcf6 into master Jul 30, 2020
@hwillson hwillson deleted the union-interface-types branch July 30, 2020 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants